Skip to content

Update webpack to v5.x#2263

Closed
walbo wants to merge 8 commits into
WordPress:trunkfrom
walbo:51750
Closed

Update webpack to v5.x#2263
walbo wants to merge 8 commits into
WordPress:trunkfrom
walbo:51750

Conversation

@walbo

@walbo walbo commented Feb 2, 2022

Copy link
Copy Markdown
Member

Trac ticket: https://core.trac.wordpress.org/ticket/51750


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Petter Walbø Johnsgård added 2 commits February 2, 2022 16:45
Comment thread tools/webpack/shared.js
@gziolo

gziolo commented Feb 4, 2022

Copy link
Copy Markdown
Member

Great work. I'll have a closer look next week. Are there more differences like the one I commented about in #2263 (comment)? I know that CopyWebpackPlugin has a different API with webpack 5, anything else that comes to your mind?

@walbo

walbo commented Feb 4, 2022

Copy link
Copy Markdown
Member Author

Great work. I'll have a closer look next week. Are there more differences like the one I commented about in #2263 (comment)? I know that CopyWebpackPlugin has a different API with webpack 5, anything else that comes to your mind?

Other than CopyWebpackPlugin, the biggest difference is the removal of @wordpress/custom-templated-path-webpack-plugin and @wordpress/library-export-default-webpack-plugin. This can now be fixed directly in the entry config.

@gziolo

gziolo commented Apr 5, 2022

Copy link
Copy Markdown
Member

@walbo, any chances to bring this PR up to date? I'd love to land that before updating WordPress packages in core for the upcoming WordPress 6.0 release.

@walbo

walbo commented Apr 5, 2022

Copy link
Copy Markdown
Member Author

@gziolo Yes, I can do that. Will try to make it happen later today, if not I'll to it tomorrow.

@gziolo

gziolo commented Apr 5, 2022

Copy link
Copy Markdown
Member

This PR will unblock #2519 which brings React Fast Refresh support to WordPress core for block development with @wordpress/scripts.

Comment thread tools/webpack/shared.js
'lodash-es': 'lodash',
},
},
stats: 'errors-only',

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why v5 shows alot more stats, but changed it to only show errors. From the logs I only see v4 logging an hash and the webpack version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include also warnings: 'errors-warnings'? Unless it's too noisy. We use the default value in Gutenberg.

Comment thread tools/webpack/shared.js
moduleIds: mode === 'production' ? 'deterministic' : 'named',
minimizer: [
new TerserPlugin( {
extractComments: false,

@walbo walbo Apr 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the License comments inside the minimized files we need to set this.

Omitting this will create a .min.LICENCE.txt file that contains the licenses. See https://webpack.js.org/plugins/terser-webpack-plugin/#extractcomments

Would be great to enable this to make the minimized files smaller, but since they are inlined today I opted to still have them inline.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we have in Gutenberg:

https://github.com/WordPress/gutenberg/blob/a618eaf35a417cd4cb7492b324cd6b8e5b86b7d9/tools/webpack/shared.js#L26-L42

I think the part for translation comments was necessary after migration to v5.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related PRs: WordPress/gutenberg#22990 and WordPress/gutenberg#28231. @swissspidy, do we need the same handling in WordPress core? We have non-minified files included, too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractComments: false makes sense.

Using the same config as Gutenberg to keep translator comments and avoid mangling i18n functions also makes sense

@walbo

walbo commented Apr 5, 2022

Copy link
Copy Markdown
Member Author

@gziolo The PR should be up to date. Added a couple of comments.

@gziolo

gziolo commented Apr 6, 2022

Copy link
Copy Markdown
Member

We can use the PR with webpack 5 upgrade in Gutenberg as a reference: WordPress/gutenberg#33818

Comment thread tools/webpack/shared.js
minimize: false,
moduleIds: 'deterministic',
};
} else if ( mode !== 'production' ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring here 👍🏻

Comment thread tools/webpack/shared.js
moduleIds: mode === 'production' ? 'deterministic' : 'named',
minimizer: [
new TerserPlugin( {
extractComments: false,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we have in Gutenberg:

https://github.com/WordPress/gutenberg/blob/a618eaf35a417cd4cb7492b324cd6b8e5b86b7d9/tools/webpack/shared.js#L26-L42

I think the part for translation comments was necessary after migration to v5.

@gziolo gziolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise it looks good. It simply needs more testing to ensure we don't break any existing workflows. I would appreciate some sanity check from someone else.

@paaljoachim

paaljoachim commented Apr 8, 2022

Copy link
Copy Markdown

I noticed that this PR is included in the WordPress 6.0 Project board.
I assume based on @gziolo comment that it should be removed from the project board. Please remove it if that is the case.

@gziolo

gziolo commented Apr 8, 2022

Copy link
Copy Markdown
Member

I noticed that this PR is included in the WordPress 6.0 Project board.
I assume based on @gziolo comment that it should be removed from the project board. Please remove it if that is the case.

Yes, it's correctly included. I asked the Build/Test Tools component maintainers to run additional checks with the intention to land it before WordPress 6.0 Beta 1.

@paaljoachim

Copy link
Copy Markdown

Thanks for the status update, Greg!

@gziolo

gziolo commented Apr 11, 2022

Copy link
Copy Markdown
Member

I'm preparing the final patch to land in WordPress core so I opened #2561 to ensure that CI checks remain green after rebasing and resolving conflicts with trunk.

@gziolo

gziolo commented Apr 11, 2022

Copy link
Copy Markdown
Member

Committed with https://core.trac.wordpress.org/changeset/53135. Major props to @walbo for making it happen! 🎉

@gziolo gziolo closed this Apr 11, 2022
@walbo walbo deleted the 51750 branch April 11, 2022 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants